-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deflake amp-form tests #9384
Deflake amp-form tests #9384
Conversation
const message = 'Failed to parse response JSON:'; | ||
user().error(TAG, message, error); | ||
rethrowAsync(message, error); | ||
user().error(TAG, `Failed to parse response JSON: ${error}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- @jridgewell
Carlos, Justin:
do we know why did we hadrethrowAsync
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My theory is it was originally there to throw the error outside of the Promise chain so it would show up in the console. Since user().error
performs that function too, it was equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
return timer.promise(5).then(() => { | ||
|
||
return ampForm.xhrSubmitPromiseForTesting().then(() => { | ||
expect(false).to.be.true; // should not run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.fail('submit should have failed.');
ditto above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I was looking for the right fail pattern, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing a plain error is concise, but assert
seems more idiomatic for Chai. Any objections to using assert
?
Fixes #9358